Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Vorwerk integration #45749

Closed
wants to merge 3 commits into from
Closed

Add Vorwerk integration #45749

wants to merge 3 commits into from

Conversation

trunneml
Copy link

@trunneml trunneml commented Jan 30, 2021

Proposed change

New Integration to bring back Vorwerk Kobold vacum cleaner support.
Before #44031 it was possible to use the neato integration.

It's possible to configure the integration by the User Config Flow in the UI with E-Mail and Code like the Vorwerk IOS app or by adding the SERIAL+SECRET_KEY of the robot in your configuration.yml. (Serial and secret can be fetched with pybotvac for example.)

VR200 is working. VR300 should work too, but without the map functionality as this needs OAuth and the current Vorwerk + Auth0 setup is somehow strange. It uses the id_token in a Auth0Bearer Authorization Header. And when calling a token refresh Python complains about the scope of the new token.

The custom_clean service supports zone cleaning. This should work with a VR300, but I can't test it as I only have the VR200.

I'm a developer, but new in writing home assistant integrations. Let me know if I should change or add something.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml
# This is optional. You can configure this integration via config flow from the ui.
vorwerk:
   - name: Mein VR
     serial: <serial>
     secret: !secret vorwerk_secret
     endpoint: <optional>

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@homeassistant
Copy link
Contributor

Hi @trunneml,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@trunneml
Copy link
Author

Hi @trunneml,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Done

@cgarwood cgarwood changed the title Vorwerk Add Vorwerk integration Jan 30, 2021
Copy link
Member

@cgarwood cgarwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start 😃 - just handful of lint errors and a few additional comments.

homeassistant/components/vorwerk/translations/de.json Outdated Show resolved Hide resolved
homeassistant/components/vorwerk/translations/en.json Outdated Show resolved Hide resolved
homeassistant/components/vorwerk/__init__.py Show resolved Hide resolved
homeassistant/components/vorwerk/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/vorwerk/vacuum.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Jan 30, 2021
@cgarwood
Copy link
Member

Tests will be needed for the config flow as well

@trunneml
Copy link
Author

trunneml commented Jan 30, 2021

Tests will be needed for the config flow as well

Where can I found a tutorial or the documentation ?

@cgarwood
Copy link
Member

Check the tests/components folder in the repo, any integration with a config flow should have a test_config_flow.py file you can take a look at 🙂

@trunneml
Copy link
Author

Check the tests/components folder in the repo, any integration with a config flow should have a test_config_flow.py file you can take a look at 🙂

Hope, I will find the time this week. :)

@trunneml
Copy link
Author

@cgarwood can you tell me whats the correct way to fix the hassfest and Check all requirements failures?

@trunneml trunneml force-pushed the vorwerk branch 2 times, most recently from 26cbacb to 8773d1d Compare January 31, 2021 15:26
@trunneml trunneml force-pushed the vorwerk branch 6 times, most recently from a5fceaa to 1dd8205 Compare February 11, 2021 17:30
@trunneml
Copy link
Author

Checks passed :)

@MartinHjelmare MartinHjelmare added this to Incoming in New Integrations Feb 23, 2021
@MartinHjelmare MartinHjelmare moved this from Incoming to Needs followup review in New Integrations Feb 23, 2021
@trunneml
Copy link
Author

@cgarwood @eavanvalkenburg Anything new? Could you merge my Pull Request? Is anything missing?

@cgarwood
Copy link
Member

cgarwood commented Aug 7, 2021

Good from my standpoint, but probably good for one of the more experienced reviewers to give it a look over.

@trunneml
Copy link
Author

@cgarwood I integrated all requested changes. But the PR is still in "Awaiting requested changes" on the https://github.com/home-assistant/core/projects/5#card-55498357 board. What is missing? How is the future process to integrate this PR?

@cgarwood cgarwood moved this from Awaiting Requested Changes to Needs followup review in New Integrations Oct 18, 2021
@rytilahti rytilahti added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Nov 8, 2021
@project-bot project-bot bot moved this from Review in progress to Second opinion wanted in Dev Nov 8, 2021
Comment on lines +1163 to +1254
homeassistant/components/vorwerk/__init__.py
homeassistant/components/vorwerk/vacuum.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not compulsory, but since new integrations can take a while to get merged, I suggest that you take the opportunity to move to 100% coverage whilst the reviews are in progress.

Suggested change
homeassistant/components/vorwerk/__init__.py
homeassistant/components/vorwerk/vacuum.py

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will improve that over Christmas.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is there to do? Just removing these lines? I want to help.

@trunneml trunneml force-pushed the vorwerk branch 2 times, most recently from 96b699d to 873c410 Compare December 26, 2021 13:36
Copy link
Member

@gjohansson-ST gjohansson-ST left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments

Comment on lines +73 to +86
async def async_setup(hass: HomeAssistantType, config: ConfigType) -> bool:
"""Set up the Vorwerk component."""
hass.data[VORWERK_DOMAIN] = {}

if VORWERK_DOMAIN in config:
hass.async_create_task(
hass.config_entries.flow.async_init(
VORWERK_DOMAIN,
context={"source": SOURCE_IMPORT},
data=config[VORWERK_DOMAIN],
)
)

return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove as not needed for new integrations

),
)

async def async_step_code(self, user_input: dict[str, Any] = None) -> FlowResult:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async def async_step_code(self, user_input: dict[str, Any] = None) -> FlowResult:
async def async_step_code(self, user_input: dict[str, Any] | None = None) -> FlowResult:

Comment on lines +72 to +79
return self.async_create_entry(
title=self._email,
data={
CONF_EMAIL: self._email,
CONF_TOKEN: self._session.token,
VORWERK_ROBOTS: robots,
},
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move outside of try

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then an extra if is needed. I don't think that makes the code more readable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is compulsory to move it out of the try block, but you can use try...except...else form

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epenet I will try that.

Comment on lines +97 to +109
async def async_step_import(self, user_input: dict[str, Any]) -> FlowResult:
"""Import a config flow from configuration."""
unique_id = "from configuration"
data = {VORWERK_ROBOTS: user_input}

await self.async_set_unique_id(unique_id)
self._abort_if_unique_id_configured(data)

_LOGGER.info("Creating new Vorwerk robot config entry")
return self.async_create_entry(
title="from configuration",
data=data,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New integration needs no import

VORWERK_ROBOT_TRAITS = "traits"
VORWERK_ROBOT_ENDPOINT = "endpoint"

VORWERK_PLATFORMS = ["vacuum"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dev automation moved this from Second opinion wanted to Review in progress Jan 6, 2022
New Integrations automation moved this from Needs followup review to Awaiting Requested Changes Jan 6, 2022
@bdraco bdraco added needs followup review and removed second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. labels Jan 17, 2022
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve conflicts. Thanks 👍

@twartzek
Copy link

twartzek commented Mar 2, 2022

Is any progress planned? I really would like to integrate the VR300 into my smart home, so I really appreciate your idea and work to integrate it.

@trunneml
Copy link
Author

trunneml commented Mar 2, 2022

Yes, but my PR is now over one year old and the feed back loop from HA is quite slow.
I'm currently very busy. I hope find the time to rebase this branch next month.
You can use my custom integration to integrated your VR300 right now (see my other GitHub repository).

@frenck
Copy link
Member

frenck commented Jun 23, 2022

@trunneml Friendly notification/ping, the review comments from @gjohansson-ST haven't been touched/implemented/processed yet and there is a merge conflict. Would you have time to pick this PR up again?

../Frenck

@trunneml
Copy link
Author

@frenck HI thanks for the reminder. I'm quite busy currently, but I will try to fix this. But after over one year to get this integration into Home-Assistant I'm questioning if this PR is the correct way.

Neato and Vorwerk Vacuum cleaners are both based on pybotvac and act the same way. The only difference is the authentication flow.
My idea was to just copy the neato integration and fix the authentication flow. But after all the reviews it's a whole new integration that has nothing to do with the neato anymore.

Please don't get me wrong all review findings were correct and the code is better now. But now it is no longer a fork of the neato integration. It is more a recode of it and that was not my intention when I started this PR.

Maybe it is a better idea to add the Vorwerk-Auth0 authentication to the neato integration. @dshokouhi and @Santober what do you think about this idea? Could you help me?

The ConfigFlow in this PR works for over one year now and I also have a working pybotvac patch, where the PasswordlessSession is based on the OAuthSession, that should work with Home Assistant like the neato OAuthSession.

What I'm not familiar with is the Home Assistant oauth2 token handling. Who could help me with that part?

@frenck
Copy link
Member

frenck commented Jan 17, 2023

Alright, considering the above response I think we can conclude this PR by itself will not be moving forwards. Therefore, I'll go ahead and close it.

../Frenck

@frenck frenck closed this Jan 17, 2023
Dev automation moved this from Review in progress to Cancelled Jan 17, 2023
New Integrations automation moved this from Awaiting Requested Changes to Cancelled Jan 17, 2023
@trunneml
Copy link
Author

@frenck: as I didn't get feedback from @dshokouhi and completly forking from neato doesn't makes sense to me, you can close this PR.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Cancelled
New Integrations
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet